fix(insights): Make HogQL queries limit_context-aware
#21022
Conversation
| @patch("posthog.hogql.query.sync_execute", wraps=sync_execute) | ||
| def test_limit_is_context_aware(self, mock_sync_execute: MagicMock): | ||
| self._run_web_overview_query("2023-12-01", "2023-12-03", limit_context=LimitContext.QUERY_ASYNC) | ||
|
|
||
| mock_sync_execute.assert_called_once() | ||
| self.assertIn(f" max_execution_time={INCREASED_MAX_EXECUTION_TIME},", mock_sync_execute.call_args[0][0]) |
There was a problem hiding this comment.
It's really tedious and brittle that we don't have a way to add a test for common mechanics to ALL query runner suites. limit_context-awareness is one thing that needs to be guaranteed for each implementation, as they implement their own calculate()s.
It looks like we could use a QueryRunnerTest base class that would inherit from ClickhouseTestMixin and have run_query() and run_actors_query(). Was there a reason for not doing that? @Gilbert09 @webjunkie
There was a problem hiding this comment.
I guess it was because of the conversion of tests basically is copying one by one the legacy tests and nobody worried about combining code or functionality yet 🙈
There was a problem hiding this comment.
👍 Cool, makes sense. I don't have the time to do this right now, but useful to do in some nearish future
| @patch("posthog.hogql.query.sync_execute", wraps=sync_execute) | ||
| def test_limit_is_context_aware(self, mock_sync_execute: MagicMock): | ||
| self._run_web_overview_query("2023-12-01", "2023-12-03", limit_context=LimitContext.QUERY_ASYNC) | ||
|
|
||
| mock_sync_execute.assert_called_once() | ||
| self.assertIn(f" max_execution_time={INCREASED_MAX_EXECUTION_TIME},", mock_sync_execute.call_args[0][0]) |
There was a problem hiding this comment.
I guess it was because of the conversion of tests basically is copying one by one the legacy tests and nobody worried about combining code or functionality yet 🙈
Problem
I found that enabling async queries did not extend the max execution time for most insight types. In fact, it only did so for SQL insights. This meant we weren't able to realize the benefits of background execution for insights.
Changes
Added
limit_contextto the relevantexecute_hogql_query()calls. This makes async queries much more powerful an escape hatch.How did you test this code?
Added a few query tests, though this is cumbersome – see the thread for more.